chore: refactor config initialisation for CLI to allow easy extension from other config sources as well MCP-288 #729
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
Pull Request Test Coverage Report for Build 19462524052Details
💛 - Coveralls |
himanshusinghs
force-pushed
the
chore/MCP-288-config-refactor
branch
from
732750d to
acfeb3c
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors configuration initialization to use yargs-parser for CLI and environment variable parsing, removes custom validation and parsing logic, and decouples connection parameters from MCPConnectionManager. It also fixes two bugs: one where positional connection string arguments were ignored without the --connectionString flag, and another where MCPConnectionManager incorrectly used configured connection's driver options for all subsequent connections.
Key Changes
- Replaced custom CLI/ENV parsing with yargs-parser
- Moved DriverOptions from MCPConnectionManager constructor to connect method
- Reorganized config code into separate modules (userConfig.ts, createUserConfig.ts, configUtils.ts)
Reviewed Changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/common/config.ts | Deleted - logic moved to separate modules |
| src/common/config/userConfig.ts | New file containing UserConfigSchema definition |
| src/common/config/createUserConfig.ts | New file with config creation and validation logic |
| src/common/config/configUtils.ts | Refactored utility functions, simplified validation |
| src/common/connectionManager.ts | Removed driverOptions parameter, now accepts ConnectionInfo in connect() |
| src/server.ts | Updated to generate ConnectionInfo when connecting |
| src/resources/common/config.ts | Updated to use generateConnectionInfoFromCliArgs |
| tests/unit/common/config.test.ts | Refactored tests to use new createUserConfig API |
| tests/integration/helpers.ts | Removed driverOptions exports and usage |
| tests/integration/tools/mongodb/connect/connect.test.ts | Added test for driver options isolation |
| All other files | Import path updates from config.js to config/userConfig.js |
This commit adds a test to assert that we don't use the driver options derived from the CLI arguments -> userConfig for further connections (other than the first connection) such as switch-connection tool. The problem earlier was that MCPConnectionManager was accepting a driverOptions object which it was then using in conjunction with userConfig to construct a fresh set of driver options for all the connections established by the MCP server. Now this was particularly problematic because the driver options for the configured connection might not be compatible with the new connection attempts being made in the MCP server. Take for example - MCP server configured to connect with an OIDC enabled MongoDB server and later switch-connection tool using the same driver options to connect to a locally running mongodb server without any auth. To fix that we've removed DriverOptions object from the interface of MCPConnectionManager and instead put it on the connect method. It is the responsibility of the caller to provide a correct ConnectionInfo object. For pre-configured connections server constructs the ConnectionInfo object using user config and passes it down to the MCPConnectionManager.connect and for the rest usage, they simply pass the connection string that they want to connect to.
Earlier we had custom code to parse ENV variable and although we were using yargs-parser to parse CLI arguments, we were still using custom logic to validate the parsed keys. This commit removes the custom code entirely in favor of using yargs-parser to parse both CLI and ENV variables while also taking care of excluding unknown arguments. This helps remove a bunch of code that we did not needed in the first place. Additionally, it fixes one of the problems with positional argument parsing. Earlier we were considering a positional argument as connection specifier only if the deprecated flag `--connectionString` was also passed. Now we consider the positional argument on top priority, disregarding ENV variable, CLI arguments, etc. This commit also splits the UserConfig schema and creation of UserConfig for CLI entry point in two different files so that the UserConfig export on the library don't end up pulling yargs-parser along.
The last refactor moved exports around and now that the UserSchema is in config/userConfig.ts, the eslint rule needed updating as well.
There is no free floating config object anymore so we don't have to worry about accidentally importing wrong config.
himanshusinghs
force-pushed
the
chore/MCP-288-config-refactor
branch
from
a7ac5c3 to
b8e490e
Compare
himanshusinghs
commented
Nov 18, 2025
Comment on lines
-18
to
-179
| export const configRegistry = z4.registry<ConfigFieldMeta>(); | ||
|
|
||
| export const UserConfigSchema = z4.object({ | ||
| apiBaseUrl: z4.string().default("https://cloud.mongodb.com/"), | ||
| apiClientId: z4 | ||
| .string() | ||
| .optional() | ||
| .describe("Atlas API client ID for authentication. Required for running Atlas tools.") | ||
| .register(configRegistry, { isSecret: true }), | ||
| apiClientSecret: z4 | ||
| .string() | ||
| .optional() | ||
| .describe("Atlas API client secret for authentication. Required for running Atlas tools.") | ||
| .register(configRegistry, { isSecret: true }), | ||
| connectionString: z4 | ||
| .string() | ||
| .optional() | ||
| .describe( | ||
| "MongoDB connection string for direct database connections. Optional, if not set, you'll need to call the connect tool before interacting with MongoDB data." | ||
| ) | ||
| .register(configRegistry, { isSecret: true }), | ||
| loggers: z4 | ||
| .preprocess( | ||
| (val: string | string[] | undefined) => commaSeparatedToArray(val), | ||
| z4.array(z4.enum(["stderr", "disk", "mcp"])) | ||
| ) | ||
| .check( | ||
| z4.minLength(1, "Cannot be an empty array"), | ||
| z4.refine((val) => new Set(val).size === val.length, { | ||
| message: "Duplicate loggers found in config", | ||
| }) | ||
| ) | ||
| .default(["disk", "mcp"]) | ||
| .describe("An array of logger types.") | ||
| .register(configRegistry, { | ||
| defaultValueDescription: '`"disk,mcp"` see below*', | ||
| }), | ||
| logPath: z4 | ||
| .string() | ||
| .default(getLogPath()) | ||
| .describe("Folder to store logs.") | ||
| .register(configRegistry, { defaultValueDescription: "see below*" }), | ||
| disabledTools: z4 | ||
| .preprocess((val: string | string[] | undefined) => commaSeparatedToArray(val), z4.array(z4.string())) | ||
| .default([]) | ||
| .describe("An array of tool names, operation types, and/or categories of tools that will be disabled."), | ||
| confirmationRequiredTools: z4 | ||
| .preprocess((val: string | string[] | undefined) => commaSeparatedToArray(val), z4.array(z4.string())) | ||
| .default([ | ||
| "atlas-create-access-list", | ||
| "atlas-create-db-user", | ||
| "drop-database", | ||
| "drop-collection", | ||
| "delete-many", | ||
| "drop-index", | ||
| ]) | ||
| .describe( | ||
| "An array of tool names that require user confirmation before execution. Requires the client to support elicitation." | ||
| ), | ||
| readOnly: z4 | ||
| .boolean() | ||
| .default(false) | ||
| .describe( | ||
| "When set to true, only allows read, connect, and metadata operation types, disabling create/update/delete operations." | ||
| ), | ||
| indexCheck: z4 | ||
| .boolean() | ||
| .default(false) | ||
| .describe( | ||
| "When set to true, enforces that query operations must use an index, rejecting queries that perform a collection scan." | ||
| ), | ||
| telemetry: z4 | ||
| .enum(["enabled", "disabled"]) | ||
| .default("enabled") | ||
| .describe("When set to disabled, disables telemetry collection."), | ||
| transport: z4.enum(["stdio", "http"]).default("stdio").describe("Either 'stdio' or 'http'."), | ||
| httpPort: z4.coerce | ||
| .number() | ||
| .int() | ||
| .min(1, "Invalid httpPort: must be at least 1") | ||
| .max(65535, "Invalid httpPort: must be at most 65535") | ||
| .default(3000) | ||
| .describe("Port number for the HTTP server (only used when transport is 'http')."), | ||
| httpHost: z4 | ||
| .string() | ||
| .default("127.0.0.1") | ||
| .describe("Host address to bind the HTTP server to (only used when transport is 'http')."), | ||
| httpHeaders: z4 | ||
| .object({}) | ||
| .passthrough() | ||
| .default({}) | ||
| .describe( | ||
| "Header that the HTTP server will validate when making requests (only used when transport is 'http')." | ||
| ), | ||
| idleTimeoutMs: z4.coerce | ||
| .number() | ||
| .default(600_000) | ||
| .describe("Idle timeout for a client to disconnect (only applies to http transport)."), | ||
| notificationTimeoutMs: z4.coerce | ||
| .number() | ||
| .default(540_000) | ||
| .describe("Notification timeout for a client to be aware of disconnect (only applies to http transport)."), | ||
| maxBytesPerQuery: z4.coerce | ||
| .number() | ||
| .default(16_777_216) | ||
| .describe( | ||
| "The maximum size in bytes for results from a find or aggregate tool call. This serves as an upper bound for the responseBytesLimit parameter in those tools." | ||
| ), | ||
| maxDocumentsPerQuery: z4.coerce | ||
| .number() | ||
| .default(100) | ||
| .describe( | ||
| "The maximum number of documents that can be returned by a find or aggregate tool call. For the find tool, the effective limit will be the smaller of this value and the tool's limit parameter." | ||
| ), | ||
| exportsPath: z4 | ||
| .string() | ||
| .default(getExportsPath()) | ||
| .describe("Folder to store exported data files.") | ||
| .register(configRegistry, { defaultValueDescription: "see below*" }), | ||
| exportTimeoutMs: z4.coerce | ||
| .number() | ||
| .default(300_000) | ||
| .describe("Time in milliseconds after which an export is considered expired and eligible for cleanup."), | ||
| exportCleanupIntervalMs: z4.coerce | ||
| .number() | ||
| .default(120_000) | ||
| .describe("Time in milliseconds between export cleanup cycles that remove expired export files."), | ||
| atlasTemporaryDatabaseUserLifetimeMs: z4.coerce | ||
| .number() | ||
| .default(14_400_000) | ||
| .describe( | ||
| "Time in milliseconds that temporary database users created when connecting to MongoDB Atlas clusters will remain active before being automatically deleted." | ||
| ), | ||
| voyageApiKey: z4 | ||
| .string() | ||
| .default("") | ||
| .describe( | ||
| "API key for Voyage AI embeddings service (required for vector search operations with text-to-embedding conversion)." | ||
| ) | ||
| .register(configRegistry, { isSecret: true }), | ||
| disableEmbeddingsValidation: z4 | ||
| .boolean() | ||
| .default(false) | ||
| .describe("When set to true, disables validation of embeddings dimensions."), | ||
| vectorSearchDimensions: z4.coerce | ||
| .number() | ||
| .default(1024) | ||
| .describe("Default number of dimensions for vector search embeddings."), | ||
| vectorSearchSimilarityFunction: z4 | ||
| .enum(similarityValues) | ||
| .default("euclidean") | ||
| .describe("Default similarity function for vector search: 'euclidean', 'cosine', or 'dotProduct'."), | ||
| previewFeatures: z4 | ||
| .preprocess( | ||
| (val: string | string[] | undefined) => commaSeparatedToArray(val), | ||
| z4.array(z4.enum(previewFeatureValues)) | ||
| ) | ||
| .default([]) | ||
| .describe("An array of preview features that are enabled."), | ||
| }); | ||
|
|
||
| export type UserConfig = z4.infer<typeof UserConfigSchema> & CliOptions; |
Collaborator
Author
There was a problem hiding this comment.
Moved to userConfig.ts as it is
himanshusinghs
commented
Nov 18, 2025
Comment on lines
-181
to
-184
| export const config = setupUserConfig({ | ||
| cli: process.argv, | ||
| env: process.env, | ||
| }); |
Collaborator
Author
There was a problem hiding this comment.
The pre-constructed config object has now been removed in favour of explicitly creating one using createUserConfig and injecting in MCP server. This helps avoiding unexpected side-effects on first import such as warnings and possibly process closure.
himanshusinghs
commented
Nov 18, 2025
Comment on lines
-186
to
-198
| export type DriverOptions = ConnectionInfo["driverOptions"]; | ||
| export const defaultDriverOptions: DriverOptions = { | ||
| readConcern: { | ||
| level: "local", | ||
| }, | ||
| readPreference: "secondaryPreferred", | ||
| writeConcern: { | ||
| w: "majority", | ||
| }, | ||
| timeoutMS: 30_000, | ||
| proxy: { useEnvironmentVariableProxies: true }, | ||
| applyProxyToOIDC: true, | ||
| }; |
Collaborator
Author
There was a problem hiding this comment.
This has been moved to ConnectionManager.connect and is only used when a connection attempt is requested without ever providing any DriverOptions.
himanshusinghs
commented
Nov 18, 2025
Comment on lines
-200
to
-264
| // Gets the config supplied by the user as environment variables. The variable names | ||
| // are prefixed with `MDB_MCP_` and the keys match the UserConfig keys, but are converted | ||
| // to SNAKE_UPPER_CASE. | ||
| function parseEnvConfig(env: Record<string, unknown>): Partial<UserConfig> { | ||
| const CONFIG_WITH_URLS: Set<string> = new Set<(typeof OPTIONS)["string"][number]>(["connectionString"]); | ||
|
|
||
| function setValue( | ||
| obj: Record<string, string | string[] | boolean | number | Record<string, unknown> | undefined>, | ||
| path: string[], | ||
| value: string | ||
| ): void { | ||
| const currentField = path.shift(); | ||
| if (!currentField) { | ||
| return; | ||
| } | ||
| if (path.length === 0) { | ||
| if (CONFIG_WITH_URLS.has(currentField)) { | ||
| obj[currentField] = value; | ||
| return; | ||
| } | ||
|
|
||
| const numberValue = Number(value); | ||
| if (!isNaN(numberValue)) { | ||
| obj[currentField] = numberValue; | ||
| return; | ||
| } | ||
|
|
||
| const booleanValue = value.toLocaleLowerCase(); | ||
| if (booleanValue === "true" || booleanValue === "false") { | ||
| obj[currentField] = booleanValue === "true"; | ||
| return; | ||
| } | ||
|
|
||
| // Try to parse an array of values | ||
| if (value.indexOf(",") !== -1) { | ||
| obj[currentField] = value.split(",").map((v) => v.trim()); | ||
| return; | ||
| } | ||
|
|
||
| obj[currentField] = value; | ||
| return; | ||
| } | ||
|
|
||
| if (!obj[currentField]) { | ||
| obj[currentField] = {}; | ||
| } | ||
|
|
||
| setValue(obj[currentField] as Record<string, string | string[] | boolean | number | undefined>, path, value); | ||
| } | ||
|
|
||
| const result: Record<string, string | string[] | boolean | number | undefined> = {}; | ||
| const mcpVariables = Object.entries(env).filter( | ||
| ([key, value]) => value !== undefined && key.startsWith("MDB_MCP_") | ||
| ) as [string, string][]; | ||
| for (const [key, value] of mcpVariables) { | ||
| const fieldPath = key | ||
| .replace("MDB_MCP_", "") | ||
| .split(".") | ||
| .map((part) => SNAKE_CASE_toCamelCase(part)); | ||
|
|
||
| setValue(result, fieldPath, value); | ||
| } | ||
|
|
||
| return result; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
The entire function has been removed. Please note that a key change is also that earlier we were expecting nested keys to be provided separated by . but that is not always possible in a shell environment and certainly not for common pattern of providing env variables. This has been noted in the user facing changes #1.
We rely on yargs-parser for all this now
himanshusinghs
commented
Nov 18, 2025
Comment on lines
-270
to
-307
| // Right now we have arguments that are not compatible with the format used in mongosh. | ||
| // An example is using --connectionString and positional arguments. | ||
| // We will consolidate them in a way where the mongosh format takes precedence. | ||
| // We will warn users that previous configuration is deprecated in favour of | ||
| // whatever is in mongosh. | ||
| function parseCliConfig(args: string[]): Partial<Record<keyof CliOptions, string | number | undefined>> { | ||
| const programArgs = args.slice(2); | ||
| const parsed = argv(programArgs, OPTIONS as unknown as argv.Options) as unknown as Record< | ||
| keyof CliOptions, | ||
| string | number | undefined | ||
| > & { | ||
| _?: string[]; | ||
| }; | ||
|
|
||
| const positionalArguments = parsed._ ?? []; | ||
|
|
||
| // we use console.warn here because we still don't have our logging system configured | ||
| // so we don't have a logger. For stdio, the warning will be received as a string in | ||
| // the client and IDEs like VSCode do show the message in the log window. For HTTP, | ||
| // it will be in the stdout of the process. | ||
| warnAboutDeprecatedOrUnknownCliArgs( | ||
| { ...parsed, _: positionalArguments }, | ||
| { | ||
| warn: (msg) => console.warn(msg), | ||
| exit: (status) => process.exit(status), | ||
| } | ||
| ); | ||
|
|
||
| // if we have a positional argument that matches a connection string | ||
| // store it as the connection specifier and remove it from the argument | ||
| // list, so it doesn't get misunderstood by the mongosh args-parser | ||
| if (!parsed.nodb && isConnectionSpecifier(positionalArguments[0])) { | ||
| parsed.connectionSpecifier = positionalArguments.shift(); | ||
| } | ||
|
|
||
| delete parsed._; | ||
| return parsed; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
The entire function is removed. It used yargs-parser but was having additional validation for config keys which is now entirely handled by yargs-parser.
himanshusinghs
commented
Nov 18, 2025
Comment on lines
-309
to
-350
| export function warnAboutDeprecatedOrUnknownCliArgs( | ||
| args: Record<string, unknown>, | ||
| { warn, exit }: { warn: (msg: string) => void; exit: (status: number) => void | never } | ||
| ): void { | ||
| let usedDeprecatedArgument = false; | ||
| let usedInvalidArgument = false; | ||
|
|
||
| const knownArgs = args as unknown as UserConfig & CliOptions; | ||
| // the first position argument should be used | ||
| // instead of --connectionString, as it's how the mongosh works. | ||
| if (knownArgs.connectionString) { | ||
| usedDeprecatedArgument = true; | ||
| warn( | ||
| "Warning: The --connectionString argument is deprecated. Prefer using the MDB_MCP_CONNECTION_STRING environment variable or the first positional argument for the connection string." | ||
| ); | ||
| } | ||
|
|
||
| for (const providedKey of Object.keys(args)) { | ||
| if (providedKey === "_") { | ||
| // positional argument | ||
| continue; | ||
| } | ||
|
|
||
| const { valid, suggestion } = validateConfigKey(providedKey); | ||
| if (!valid) { | ||
| usedInvalidArgument = true; | ||
| if (suggestion) { | ||
| warn(`Warning: Invalid command line argument '${providedKey}'. Did you mean '${suggestion}'?`); | ||
| } else { | ||
| warn(`Warning: Invalid command line argument '${providedKey}'.`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (usedInvalidArgument || usedDeprecatedArgument) { | ||
| warn("- Refer to https://www.mongodb.com/docs/mcp-server/get-started/ for setting up the MCP Server."); | ||
| } | ||
|
|
||
| if (usedInvalidArgument) { | ||
| exit(1); | ||
| } | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Removed and being handled differently now. All the warnings are still maintained though.
himanshusinghs
commented
Nov 18, 2025
Comment on lines
-352
to
-373
| export function registerKnownSecretsInRootKeychain(userConfig: Partial<UserConfig>): void { | ||
| const keychain = Keychain.root; | ||
|
|
||
| const maybeRegister = (value: string | undefined, kind: Secret["kind"]): void => { | ||
| if (value) { | ||
| keychain.register(value, kind); | ||
| } | ||
| }; | ||
|
|
||
| maybeRegister(userConfig.apiClientId, "user"); | ||
| maybeRegister(userConfig.apiClientSecret, "password"); | ||
| maybeRegister(userConfig.awsAccessKeyId, "password"); | ||
| maybeRegister(userConfig.awsIamSessionToken, "password"); | ||
| maybeRegister(userConfig.awsSecretAccessKey, "password"); | ||
| maybeRegister(userConfig.awsSessionToken, "password"); | ||
| maybeRegister(userConfig.password, "password"); | ||
| maybeRegister(userConfig.tlsCAFile, "url"); | ||
| maybeRegister(userConfig.tlsCRLFile, "url"); | ||
| maybeRegister(userConfig.tlsCertificateKeyFile, "url"); | ||
| maybeRegister(userConfig.tlsCertificateKeyFilePassword, "password"); | ||
| maybeRegister(userConfig.username, "user"); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
This has been maintained in createUserConfig.ts
himanshusinghs
commented
Nov 18, 2025
Comment on lines
-375
to
-391
| export function warnIfVectorSearchNotEnabledCorrectly(config: UserConfig, warn: (message: string) => void): void { | ||
| const vectorSearchEnabled = config.previewFeatures.includes("vectorSearch"); | ||
| const embeddingsProviderConfigured = !!config.voyageApiKey; | ||
| if (vectorSearchEnabled && !embeddingsProviderConfigured) { | ||
| warn(`\ | ||
| Warning: Vector search is enabled but no embeddings provider is configured. | ||
| - Set an embeddings provider configuration option to enable auto-embeddings during document insertion and text-based queries with $vectorSearch.\ | ||
| `); | ||
| } | ||
|
|
||
| if (!vectorSearchEnabled && embeddingsProviderConfigured) { | ||
| warn(`\ | ||
| Warning: An embeddings provider is configured but the 'vectorSearch' preview feature is not enabled. | ||
| - Enable vector search by adding 'vectorSearch' to the 'previewFeatures' configuration option, or remove the embeddings provider configuration if not needed.\ | ||
| `); | ||
| } | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
This has been maintained in createUserConfig.ts
himanshusinghs
commented
Nov 18, 2025
Comment on lines
-393
to
-430
| export function setupUserConfig({ cli, env }: { cli: string[]; env: Record<string, unknown> }): UserConfig { | ||
| const rawConfig = { | ||
| ...parseEnvConfig(env), | ||
| ...parseCliConfig(cli), | ||
| }; | ||
|
|
||
| if (rawConfig.connectionString && rawConfig.connectionSpecifier) { | ||
| const connectionInfo = generateConnectionInfoFromCliArgs(rawConfig as UserConfig); | ||
| rawConfig.connectionString = connectionInfo.connectionString; | ||
| } | ||
|
|
||
| const parseResult = UserConfigSchema.safeParse(rawConfig); | ||
| if (parseResult.error) { | ||
| throw new Error( | ||
| `Invalid configuration for the following fields:\n${parseResult.error.issues.map((issue) => `${issue.path.join(".")} - ${issue.message}`).join("\n")}` | ||
| ); | ||
| } | ||
| // We don't have as schema defined for all args-parser arguments so we need to merge the raw config with the parsed config. | ||
| const userConfig = { ...rawConfig, ...parseResult.data } as UserConfig; | ||
|
|
||
| warnIfVectorSearchNotEnabledCorrectly(userConfig, (message) => console.warn(message)); | ||
| registerKnownSecretsInRootKeychain(userConfig); | ||
| return userConfig; | ||
| } | ||
|
|
||
| export function setupDriverConfig({ | ||
| config, | ||
| defaults, | ||
| }: { | ||
| config: UserConfig; | ||
| defaults: Partial<DriverOptions>; | ||
| }): DriverOptions { | ||
| const { driverOptions } = generateConnectionInfoFromCliArgs(config); | ||
| return { | ||
| ...defaults, | ||
| ...driverOptions, | ||
| }; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Removed as these were not needed anymore
Last change missed using default driver options for connection info without driver options. We are bringing that back now. Also the ensureConnected is modified to use correct connectionInfo.
gagik
reviewed
Nov 18, 2025
gagik
approved these changes
Nov 18, 2025
1. Use eslint disable instead of void marking the unused expression. 2. Make deprecatedCliArgumentWarnings a string instead of list of strings
Creates a dedicated method on Session to connect to the connection configured in the config. This involves generating proper connection info.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
This PR proposes the following:
driverOptionsfrom the MCPConnectionManager constructor and moving it instead ofMCPConnectionManager.connectFurthermore this PR fixes two accidental bugs:
--connectionStringflag was also provided which defeats the purpose of warning against using--connectionString. The final effect of this was that the value of--connectionStringwould have taken hold and MCP server will try connecting to that instead of the one specified in positional argument.DriverOptionsof the configured connection in the user config due to which it could never be guaranteed forswitch-connectiontool to connect successfully even when the connection string would've been correct. The problem was apparent when the driver options used by configured connection was not compatible with the connection to be established by switch-connection tool. For example - consider configuring an OIDC connection, this would've made MCPConnectionManager bound to use configuredDriverOptionfor any further connection and there is no guarantee that the next connection attempted by the MCPConnectionManager, lets say via switch-connection tool, would've been an OIDC connection with exact same parameters.This opens path for simply using yargs-parser to also parse a configuration file in the correct order or priority.
Notes for reviewer:
config.ts. The overall idea here was to:1.1 Remove unnecessary code - validation logic, env and cli variable parsing logic. So you will see them removed and most of the work being done instead with yargs-parser.
1.2 Separate different entities in different readable modules so you will see that UserConfigSchema is moved to its own file and the methods creating UserConfig are moved to separate files. More than just aesthetic, this also prevents yargs-parser accidentally leaking on the lib.
Notable user facing changes:
.notation now we use__instead (default from yargs-parser). Rationale behind this is that.notation in env variables do not work for normal usage such as passing env variable inline to command or setting them usingexport. So we're essentially fixing another odd behaviour. Good thing is that we did not have any nested config key that needed parsing of nested ENV variable.Checklist